Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] Allow constructing Quat from two Vector3s #90464

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

timothyqiu
Copy link
Member

@timothyqiu timothyqiu commented Apr 10, 2024

Closes godotengine/godot-proposals#424

This quaternion constructor is exposed in 4.x.

But GDScript in 3.x doesn't allow having multiple constructors with the same argument count but different types. Nor are static methods available. Therefore, I added the quat_to() method to Vector3, similar to angle_to().

Adds set_rotation(from, to) to Quat.

AThousandShips
AThousandShips previously approved these changes Apr 10, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Apr 10, 2024
@lawnjelly
Copy link
Member

lawnjelly commented Apr 10, 2024

Could this be a method of Quat instead of Vector3?

It seems slightly counter-intuitive (and difficult for discoverability) to try and put this as a method of Vector3. 🤔

@AThousandShips : Note this is for 3.x, as 4.x has it exposed already it sounds like, but 3.x can not use the same method.

@lawnjelly lawnjelly modified the milestones: 4.x, 3.x Apr 10, 2024
@AThousandShips
Copy link
Member

Yep, realized after, you caught it before I got to it

@timothyqiu
Copy link
Member Author

timothyqiu commented Apr 10, 2024

Could this be a method of Quat instead of Vector3?

Could be. The downside is that you have to create an empty Quat first, as there is no static method. Do you think it's acceptable?

var q1 := v1.quat_to(v2)

var q2 := Quat()
q2.make_rotation(v1, v2)

@lawnjelly
Copy link
Member

lawnjelly commented Apr 10, 2024

Could be. The downside is that you have to create an empty Quat first, as there is no static method. Do you think it's acceptable?

Hmm, certainly worth getting a few more opinions on before merging (I'm open to either, but would be nice to be sure before committing to an API). In fact I'd prefer to leave this for @akien-mga to look at before we commit to it.

I'll admit the former looks more efficient (in e.g. bound languages with no optimization), but on the other hand it seems to make more logical sense to keep Quat methods with Quat rather than moving to Vector3.

@AThousandShips AThousandShips dismissed their stale review April 10, 2024 08:49

Awaiting possible new solution

@akien-mga
Copy link
Member

I think moving it to Quat makes sense, even though the lack of static method is a bit awkward.

The way the method is now defined in the middle of variant_call.cpp instead of relying on VCALL_ macros to use the member function seems to also advocate for a less hacky approach.

@timothyqiu
Copy link
Member Author

Changed to Quat.set_rotation(from, to).

Just noticed that Quat already has similar methods set_euler(euler) and set_axis_angle(axis, angle). So I adapted the naming.

core/math/quat.h Outdated Show resolved Hide resolved
@lawnjelly
Copy link
Member

The name set_rotation() seems a little of a cop out, given that quaternions are usually rotations, and there are multiple ways of setting them (e.g. axis angle etc).

Would set_shortest_arc() or something like that be any better? 🤔

@lawnjelly
Copy link
Member

I think shortest arc is probably the best term. There is also "great circle arc" and "minor arc" but shortest arc seems to convey what we are after.

Double checking with any mathematicians, @kleonc does this naming sound okay?

@kleonc
Copy link
Member

kleonc commented Jun 12, 2024

Double checking with any mathematicians, @kleonc does this naming sound okay?

Sounds fine to me, but got to clarify it n-th time: I'm not a mathematician. 😄 I do get why I could be considered a math-guy though, and I'm absolutely fine with it! Still had to clarify. 🙂

Besides the naming I want you to be aware of #80249, see my comment: #80249 (comment) (seems still relevant).

To be sure I'll link this / ask for some potential feedback on Godot Discord's math channel (there are some more-mathy-than-me folks in there).

@lawnjelly
Copy link
Member

Besides the naming I want you to be aware of #80249, see my comment: #80249 (comment) (seems still relevant).

Yes the input could be verified (at the least in DEV_ENABLED). And the output is undefined in the case of 180 degrees. We've had issues on this in the past if I remember right:
#53928

I wonder whether we should return a bool for success or failure to cover this, or an error code. There is an opportunity to make a more sensible function than is present in 4.x.

e.g.

OK
ERR_PARAMETER_RANGE_ERROR
FAILED

What do you guys think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants